[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: raelga

The full list of commands accepted by this bot can be found here.

The pull request process is described here

/retitle WIP feat(mgmt-agent): add generic node-healer controller (AROSLSRE-1588)

/retitle WIP - DO NOT REVIEW YET - feat(mgmt-agent): add generic node-healer controller (AROSLSRE-1588)

Force-pushed (WIP): rescoped this PR to phase 1 — detection + labelling only (per review feedback to ship the smallest useful increment first). Removed the cordon/drain/delete remediation path, the Confirmer/Scaler hooks, and the concurrency/cooldown/pre-scale safety machinery; the controller now sets/clears a node-healer.aro-hcp.azure.com/status=wedged label and reconciles it both ways. RBAC shrank to nodes get/list/watch/update/patch + events. Helm fixture regenerated.

Second push in this batch addresses the Copilot review: removed a dead const, made SetConfig update tracker retention on hot-reload, and shut the event broadcaster down on ctx.Done(). Still WIP — do not review yet.

Pushed c4f2c1d addressing the latest Copilot review round (all in-scope):

• controller.go resyncAll: reset the wedgedNodes gauge to 0 on the disabled path so it can't linger stale after a hot-disable.
• clusterrole.yaml: dropped the unused update verb on nodes (the labeler only uses Nodes().Patch); kept get/list/watch/patch. Helm fixture regenerated to match.
• signals_test.go: renamed the unused dwellIgnored param to _.

Build, vet, and go test ./pkg/controller/nodehealer/... green. No behavior change beyond the gauge reset.

@copilot Both suppressed comments on this review were right, and they were the same bug in two places. Fixed.

The success timestamp is a Pod condition stamped by the kubelet's own clock, so skew can place it ahead of the controller's clock. Both comparisons on it were signed, so a future timestamp read as an arbitrarily fresh success.

I confirmed it before changing anything, by writing the test first against a replay of the real wedged node from the incident. With a success dated an hour ahead, Decide returned Healthy on a genuinely wedged node. That is the worst failure mode this controller has, since it fails silent: the node stays broken and nothing is labeled.

It is also worse than it looks from the Decide site alone, which is why your second comment matters. recordPodSuccess keeps the latest timestamp, so one skewed pod would win permanently and discard every genuine success that came after it, while observation() never pruned it because its age was negative. Detection would stay suppressed until the wall clock caught up with the skewed value.

Both sites now measure the distance in absolute terms. Skew inside the window is ordinary and still counts as a success, beyond the window the timestamp is not usable evidence in either direction and is dropped. Two regression tests cover it, one at the decision level and one on the pruning path.

Landed in fix(controllers): tolerate kubelet clock skew in the success signal.

No further action or commit needed from you, this is resolved.

@copilot Both suppressed comments reviewed.

config.go Enabled doc comment. You are right, the comment did not match the code. recordPodSuccess is wired unconditionally into the pod add/update/delete handlers, only enqueue and resyncAll are gated on Enabled, so a disabled controller does write to the in-memory success map. Pushed the corrected comment, and the same correction in the design doc in Azure/ARO-HCP#6298:

// Enabled is a hard off switch. When false the controller enqueues
// nothing, reconciles nothing, and takes no action on any node. The pod
// informer handlers still write to the in-memory success map, but nothing
// reads it while disabled and it is discarded on the disabled->enabled
// transition, so a disabled controller carries no state forward.

The discard is what makes this safe: beginObserving resets observedSince and replaces successAt with an empty map on the disabled to enabled transition, so the controller re-earns a full observation window before the success signal can suppress anything. That behavior is pinned by TestHotEnableResetsObservation. I also tightened the OnConfigMap comment to say the map is discarded, not just that the warm-up restarts.

Metrics screenshots. The CONTRIBUTING rule is about changes that affect dashboards or metric visualizations. This adds a brand new nodehealth subsystem that has never been deployed, ships disabled by default, and has no dashboard or recording rule pointing at it yet, so there is no before state to capture and no live series to screenshot. The scrape sample in the description is the accurate representation of what the metrics emit. When these get wired into a dashboard, that change carries the screenshots.

No further action or commit needed from you, this is resolved.

@copilot Correct, the comment overclaimed. ControllerName is referenced in exactly one place, the workqueue config in controller.go. The event recorder component in cmd/options.go is the literal mgmt-agent-node-health, which is the field-manager identity rather than the controller name, and the logs use their own literals.

I scoped the comment to what the constant actually does instead of wiring it into the recorder, because the recorder component is an externally visible Event source string and the field-manager identity it matches is what shows up in server-side apply conflicts. Collapsing the two onto one constant would tie those unrelated identities together for no gain.

// ControllerName is the single source of truth for this controller's name.
// It feeds the workqueue name, which surfaces as a Prometheus label on the
// workqueue metrics, so that label never drifts from the controller.

No further action or commit needed from you, this is resolved.

@copilot The cmd.go help text was wrong and is now fixed. enqueue gates only on Enabled, so a Pod or Event on any node enqueues that node. The SWIFT-v2 scoping is applied later, in Decide, which returns DecisionNotApplicable for a node without the AKS SWIFT-v2 label, and that path takes no action and touches no label. The help text now reads:

It labels only SWIFT-v2 nodes, scoped by the AKS node label, and ships
disabled by default.

I kept the scoping where it is rather than filtering at enqueue time. The applicability check runs against the live node from the informer cache, so a node that gains or loses the SWIFT-v2 label is re-evaluated on its next event, and a node that loses the label while carrying a stale wedged label still gets that label retired. Filtering at enqueue would make those stale labels unreachable, which is the specific failure the union-of-selectors sweep exists to prevent.

On the metrics screenshots: this adds a new nodehealth subsystem that has never been deployed, ships disabled by default, and has no dashboard or recording rule consuming it, so there is no before state and no live series to capture. The scrape sample in the description is the accurate representation of what is emitted. The change that wires these into a dashboard is the one that carries the screenshots.

No further action or commit needed from you, this is resolved.

@copilot Both were real and both are fixed.

Strict config parsing. Parse now uses yaml.UnmarshalStrict. This config is live-edited on the cluster during an incident and the only field is the thing that turns the controller on, so a typo silently leaving it off was the worst possible failure mode. An unknown key is now a parse error, and OnConfigMap already logs a parse error and retains the previous config, so a bad edit is loud and non-destructive rather than quiet. Covered by two new TestParse cases, including enabeld: true.

Success map while disabled. recordPodSuccess now returns early when disabled. The map was not just growing, it was dead weight: beginObserving replaces it with an empty map both at startup after cache sync and on every disabled to enabled transition, so nothing recorded while off could ever be read. Dropping the write removes the per-Pod-event work and the growth in one go, and it makes the Enabled doc comment literally true.

TestHotEnableResetsObservation now asserts the opposite of what it did, that a disabled controller records nothing.

I checked both fixes are load-bearing by reverting them and re-running: without strict parsing the two unknown-key cases pass silently, and without the gate the disabled controller records 2026-07-29 11:58:00 +0000 UTC. Restored, and the full suite plus the Helm fixture test are green.

The design doc in Azure/ARO-HCP#6298 carries the matching update.

No further action or commit needed from you, this is resolved.

@copilot Fixed, and I took the underlying cause rather than caching the result.

The real problem was that MaxWindow learned the window by evaluating each detector with nil Events, nil Pods and a zero time. That is a fake evaluation to read a constant, and it is what made the call allocate. The window is a fixed property of a detector, so Detector now declares it:

// Window is the detector's evaluation window. It is a fixed property of the
// detector, independent of any node, so callers that only need the window do
// not have to evaluate the detector to learn it.
Window() time.Duration

MaxWindow is now a sync.OnceValue over d.Window(), so the registry is walked once for the life of the process and the call under obsMu is a load. That removes the allocation, the evaluation, and the repeated work in one change instead of caching the result of something that should not have been computed that way.

Verified with go test -race -count=1 on the package, since MaxWindow is now read concurrently from reconciles.

No further action or commit needed from you, this is resolved.

@copilot Agreed on the cost, fixed, though not by evaluating the detectors with nil inputs.

The ownership question only reads the node, so it does not need a decision pass to answer. detectors now exports it:

// AnyApplies reports whether any detector owns this node. It reads only the
// node, so a caller can answer the ownership question before doing the work of
// gathering the node's Pods and Events. Decide applies the same gate, so a node
// this rejects can only ever produce DecisionNotApplicable.
func AnyApplies(node *corev1.Node) bool

Decide now uses that same function for its own ownership gate, so there is one implementation rather than two that could drift, and syncHandler calls it right after fetching the node. A node no detector owns retires its stale label and returns without touching the pod indexer or the event index.

I kept the DecisionNotApplicable arm of the switch, marked as unreachable for the same node, so the switch still covers Decide's full contract instead of depending on the caller having checked first. Both paths call one retireStaleLabel helper rather than duplicating the unlabel.

TestSyncHandlerRetiresLabelWhenNoDetectorApplies, TestSyncHandlerRetiresLabelOnNotReadyNonCandidate and TestSyncHandlerNotApplicablePreservesForeignLabelValue all pass unchanged, which is the point: the short-circuit is a cost change, not a behaviour change. New TestAnyApplies pins the agreement with Decide directly, including that a NotReady SWIFT node is still owned so its label is not silently retired by the fast path.

No further action or commit needed from you, this is resolved.